StandardToken encapsulation #1197

frangio · 2018-08-12T20:39:21Z

Fixes #1097.
Fixes #1208.

Makes all state variables of StandardToken private, and adds internal functions to modify them correctly emitting events.

nventuro · 2018-08-12T23:05:15Z

contracts/token/ERC20/MintableToken.sol

    emit Mint(_to, _amount);
-    emit Transfer(address(0), _to, _amount);
+    _mint(_to, _amount);


Shouldn't the event be emitted after the action took place, according to our guideline?

Ah, yes. I initially did it like this because the test was relying on the previous order: Mint followed by Transfer. I'll fix the test.

This reverts commit 371fe3e.

frangio · 2018-08-13T02:07:51Z

Addressed previous comments and added tests for the new functions. @nventuro Please review again! 🙂

nventuro

I noticed you skipped some of the requires for which we already have corresponding ones (e.g. transferFrom and burnFrom), leaving SafeMath as the only line of defense. Is this something we want to do?

nventuro · 2018-08-13T11:59:49Z

contracts/token/ERC20/BurnableToken.sol

-
-    balances[_who] = balances[_who].sub(_value);
-    totalSupply_ = totalSupply_.sub(_value);
+    super._burn(_who, _value);


This is confusing: the public burn function calls the internal _burn function, which then calls super._burn and emits an event. Why not remove the internal _burn and just call base function from the public one?

edit: you did it right in the mint function, so I'm assuming this wasn't intentional :)

I did this because both burn and burnFrom need to add the Burn event. I felt it was better than adding the event in both. What do you think?

That's even worse now that I realize it lol. burnFrom calls _burnFrom, which calls the overriden _burn, which calls the original burn: unless you know the details of StandardToken, you can't tell that burnFrom will emit a Burn event. The reason why you did it makes sense though.

I think the problem is the missing comment: if the overridden function said something like 'adds the Burn event when burning', it may read more clearly.

nventuro · 2018-08-13T12:02:20Z

contracts/token/ERC20/StandardToken.sol

+   * @param _amount The amount that will be created.
+   */
+  function _mint(address _account, uint256 _amount) internal {
+    totalSupply_ = totalSupply_.add(_amount);


Input validation on amount > 0 and account != 0?

_amount > 0 is a validation that we have decided in the past to not do, because it forces users of the function to special-case amount == 0 to avoid triggering a revert.

_account != 0 I guess we should add.

nventuro · 2018-08-13T12:02:48Z

contracts/token/ERC20/StandardToken.sol

+   * @param _amount The amount that will be burnt.
+   */
+  function _burn(address _account, uint256 _amount) internal {
+    totalSupply_ = totalSupply_.sub(_amount);


Input validation on amount > 0, account != 0, and balances[account] > amount?

I'm not sure about balances[account] > amount. I thought we were using SafeMath for this kind of thing, now that it uses require.

Is this something we want to do?

I'm not sure.

nventuro · 2018-08-13T12:04:34Z

contracts/token/ERC20/StandardToken.sol

+  function _burnFrom(address _account, uint256 _amount) internal {
+    // Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted,
+    // this function needs to emit an event with the updated approval.
+    allowed[_account][msg.sender] = allowed[_account][msg.sender].sub(_amount);


Input validation on amount > 0, account != 0, balances[account] > amount and allowance[acount][msg.sender] > amount?

barakman · 2018-08-14T13:37:22Z

Are you sure you want to declare private stuff in your contracts?
They are generally inherited by (I assume) every user out there, so internal feels much more appropriate here (in any any other base contract for that matter).

nventuro · 2018-08-14T13:48:22Z

@barakman if the variables are private, auditing becomes that much easier, because you can be sure all accesses to it are restricted to the current contract. Additionally, it prevents accidental mistakes (e.g. adding balance to a user but forgetting to update totalSupply). See how the _mint and _burn internal functions give access to that functionality, but in a safe manner.

frangio · 2018-08-14T21:31:23Z

@nventuro Fixed new comments. I added in all the checks for balance and allowance, though I'd consider removing all those in a different issue.

I'm requesting another review since this is a breaking change.

shrugs · 2018-08-15T02:37:37Z

contracts/token/ERC20/StandardToken.sol

+   * @param _amount The amount that will be burnt.
+   */
+  function _burnFrom(address _account, uint256 _amount) internal {
+    require(_account != 0);


this line and the last require are duplicate assertions since we're falling back to _burn. Would save some gas, presumably, if we omit them but mention this in a comment.

shrugs · 2018-08-15T02:38:57Z

test/token/ERC20/MintableToken.behavior.js

+            const mintEvent = expectEvent.inLogs(logs, 'Mint', {
+              to: owner,
+            });
+            mintEvent.args.amount.should.be.bignumber.equal(amount);


you can actually put that in the args object as well:

const mintEvent = expectEvent.inLogs(logs, 'Mint', { to: owner, amount, });

and it will do the bignumber equals check. likewise below

Are you sure about this? The code for the helper looks like it uses JavaScript's equality operator.

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/8d11dcc0e5fb0fd50e018d7786750fa45e54d4c5/test/helpers/expectEvent.js#L6-L9

This was actually one of the items @nventuro mentioned in #1026.

shrugs · 2018-08-15T19:46:02Z

Ah I totally never merged that change 😢. I guess this can be refactoring back in a later PR.

…

On Wed, Aug 15, 2018, 12:09 Francisco Giordano ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/token/ERC20/MintableToken.behavior.js <#1197 (comment)> : > @@ -104,11 +107,16 @@ function shouldBehaveLikeMintableToken (owner, minter, [anyone]) { it('emits a mint and a transfer event', async function () { const { logs } = await this.token.mint(owner, amount, { from }); - logs.length.should.eq(2); - logs[0].event.should.eq('Mint'); - logs[0].args.to.should.eq(owner); - logs[0].args.amount.should.be.bignumber.equal(amount); - logs[1].event.should.eq('Transfer'); + const mintEvent = expectEvent.inLogs(logs, 'Mint', { + to: owner, + }); + mintEvent.args.amount.should.be.bignumber.equal(amount); Are you sure about this? The code code for the helper looks like it uses JavaScript's equality operator. https://github.com/OpenZeppelin/openzeppelin-solidity/blob/8d11dcc0e5fb0fd50e018d7786750fa45e54d4c5/test/helpers/expectEvent.js#L6-L9 — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1197 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsGbUz9WhLz_57yBks8NNHm-h8o83dks5uRHHkgaJpZM4V5req> .

frangio · 2018-08-15T20:12:32Z

Addressed all comments! Can I get a ✔️? 😬

shrugs

LGTM

nventuro

Could you double check CappedToken.behavior.js? It looks like there may be whitespace changes on that one (probably the line endings).

frangio · 2018-08-16T14:28:42Z

@nventuro You were right. I had to force push that last merge commit which dismissed the reviews.

frangio · 2018-08-16T16:03:35Z

Thanks @nventuro and @shrugs! 😄

frangio added 4 commits August 12, 2018 13:35

make StandardToken state variables private

cb75f00

simplify mocks

371fe3e

document new internal functions

4762191

fix link to ERC20 document

5f89cdf

frangio requested a review from nventuro August 12, 2018 20:39

frangio mentioned this pull request Aug 12, 2018

All state variables should be private #1174

Closed

frangio self-assigned this Aug 12, 2018

nventuro reviewed Aug 12, 2018

View reviewed changes

frangio added 3 commits August 12, 2018 21:24

revert order of Transfer and Mint events

8120c46

Revert "simplify mocks"

dc8b3bc

This reverts commit 371fe3e.

add tests for new internal functions

2cd86bf

frangio added kind:improvement contracts Smart contract code. breaking change Changes that break backwards compatibility of the public API. feature New contracts, functions, or helpers. labels Aug 13, 2018

Merge branch 'master' into erc20-encapsulation

6b3f6d6

nventuro suggested changes Aug 13, 2018

View reviewed changes

nventuro mentioned this pull request Aug 14, 2018

Visibility of StandardToken state variables #1208

Closed

frangio added 2 commits August 14, 2018 18:30

add check for null account

53f7b0e

add checks for balances and allowance

bc88f3c

frangio requested a review from shrugs August 14, 2018 21:31

frangio added this to the v2.0 milestone Aug 14, 2018

frangio mentioned this pull request Aug 15, 2018

Adding tokens/ERC20/PreserveBalancesOnTransferToken.sol #1100

Closed

4 tasks

shrugs suggested changes Aug 15, 2018

View reviewed changes

frangio added 2 commits August 15, 2018 16:02

add inline docs to BurnableToken._burn

78f4b9e

remove redundant checks and clarify why

34a967b

shrugs previously approved these changes Aug 15, 2018

View reviewed changes

nventuro reviewed Aug 16, 2018

View reviewed changes

nventuro previously approved these changes Aug 16, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into erc20-encapsulation

24cb9cf

frangio dismissed stale reviews from nventuro and shrugs via 24cb9cf August 16, 2018 14:25

frangio force-pushed the erc20-encapsulation branch from 51ca533 to 24cb9cf Compare August 16, 2018 14:25

nventuro approved these changes Aug 16, 2018

View reviewed changes

frangio merged commit 4dcdd29 into OpenZeppelin:master Aug 16, 2018

frangio deleted the erc20-encapsulation branch August 16, 2018 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StandardToken encapsulation #1197

StandardToken encapsulation #1197

frangio commented Aug 12, 2018 •

edited by nventuro

nventuro Aug 12, 2018

frangio Aug 12, 2018

frangio commented Aug 13, 2018 •

edited

nventuro left a comment

nventuro Aug 13, 2018 •

edited

frangio Aug 14, 2018

nventuro Aug 14, 2018

nventuro Aug 13, 2018 •

edited

frangio Aug 14, 2018 •

edited

nventuro Aug 13, 2018 •

edited

frangio Aug 14, 2018

nventuro Aug 13, 2018 •

edited

barakman commented Aug 14, 2018

nventuro commented Aug 14, 2018

frangio commented Aug 14, 2018

shrugs Aug 15, 2018

shrugs Aug 15, 2018

frangio Aug 15, 2018 •

edited

shrugs commented Aug 15, 2018 via email

frangio commented Aug 15, 2018

shrugs left a comment

nventuro left a comment

frangio commented Aug 16, 2018

frangio commented Aug 16, 2018

StandardToken encapsulation #1197

StandardToken encapsulation #1197

Conversation

frangio commented Aug 12, 2018 • edited by nventuro

nventuro Aug 12, 2018

Choose a reason for hiding this comment

frangio Aug 12, 2018

Choose a reason for hiding this comment

frangio commented Aug 13, 2018 • edited

nventuro left a comment

Choose a reason for hiding this comment

nventuro Aug 13, 2018 • edited

Choose a reason for hiding this comment

frangio Aug 14, 2018

Choose a reason for hiding this comment

nventuro Aug 14, 2018

Choose a reason for hiding this comment

nventuro Aug 13, 2018 • edited

Choose a reason for hiding this comment

frangio Aug 14, 2018 • edited

Choose a reason for hiding this comment

nventuro Aug 13, 2018 • edited

Choose a reason for hiding this comment

frangio Aug 14, 2018

Choose a reason for hiding this comment

nventuro Aug 13, 2018 • edited

Choose a reason for hiding this comment

barakman commented Aug 14, 2018

nventuro commented Aug 14, 2018

frangio commented Aug 14, 2018

shrugs Aug 15, 2018

Choose a reason for hiding this comment

shrugs Aug 15, 2018

Choose a reason for hiding this comment

frangio Aug 15, 2018 • edited

Choose a reason for hiding this comment

shrugs commented Aug 15, 2018 via email

frangio commented Aug 15, 2018

shrugs left a comment

Choose a reason for hiding this comment

nventuro left a comment

Choose a reason for hiding this comment

frangio commented Aug 16, 2018

frangio commented Aug 16, 2018

frangio commented Aug 12, 2018 •

edited by nventuro

frangio commented Aug 13, 2018 •

edited

nventuro Aug 13, 2018 •

edited

nventuro Aug 13, 2018 •

edited

frangio Aug 14, 2018 •

edited

nventuro Aug 13, 2018 •

edited

nventuro Aug 13, 2018 •

edited

frangio Aug 15, 2018 •

edited